feat(nextjs): add firebase-cookie-middleware with security hardening by tyler-reitz · Pull Request #739 · FirebaseExtended/reactfire

tyler-reitz · 2026-07-16T22:40:59Z

Summary

This PR introduces firebase-cookie-middleware (v0.0.1), a production-ready Next.js middleware package for Firebase Auth cookie persistence. It takes over from #640 (original work by James Daniels, @jamesdaniels), which is being closed in favor of this PR to establish clear ownership.

All logic from #640 has been carried forward. The following security issues were identified and addressed during the takeover review:

Emulator explicit opt-in: FIREBASE_AUTH_EMULATOR_HOST is no longer auto-detected; requires emulator: true in config to prevent unsigned token acceptance if the env var leaks into production
Verify-after-refresh: refreshed ID tokens are verified before being accepted, preventing a bypass via a compromised token endpoint
refreshable=false on signature failure: prevents refresh attempts when the original token has an invalid signature (not just expiry)
SSRF: emulator host validated to be loopback-only (localhost, 127.0.0.1, ::1)
CSRF: Origin header checked on POST/DELETE to /__cookies__; cross-origin requests rejected with 403
CHIPS: SameSite=None applied correctly on partitioned cookies (was incorrectly SameSite=Lax in production)
JWKS eviction lock: lock released in a finally block to prevent deadlock on verification error
Falsy-zero TTL: ttlSeconds > 0 required before caching; 0 no longer treated as "cache forever"
400 not throw: invalid proxy target hosts return 400 instead of an uncaught throw
Optional chaining on JWT payload: guards against undefined access on malformed tokens
Removed duplex: 'half': workaround was unnecessary and caused Node.js type errors

Next.js 16 compatibility: adds a proxy named export for proxy.ts (Node.js runtime). The middleware export remains for middleware.ts users (Edge Runtime, v14/15 and v16 deprecated path). Logic is identical between the two exports.

Also fixes pre-existing TypeScript strict-mode errors surfaced by upgrading moduleResolution from node to bundler.

Test plan

71 unit tests pass (npm test in src/nextjs/)
tsc --noEmit clean at repo root and in src/nextjs/
Manual smoke test: emulator flow with emulator: true + FIREBASE_AUTH_EMULATOR_HOST set
Manual smoke test: production token verify and refresh cycle in a Next.js 15 app
Manual smoke test: proxy.ts export in a Next.js 16 app

Attribution

Original implementation by James Daniels (jamesdaniels@google.com), PR #640. Takeover and security hardening by Tyler Dixon.

Closes #640

gemini-code-assist · 2026-07-16T22:43:54Z

Warning

Gemini encountered an error creating the review. You can try again by commenting /gemini review.

tyler-reitz · 2026-07-16T22:49:20Z

/gemini review

gemini-code-assist · 2026-07-16T22:51:02Z

Warning

Gemini encountered an error creating the review. You can try again by commenting /gemini review.

- Move jose from devDependencies to dependencies (runtime dep) - Strip API key from refresh error log (log origin+pathname only) - Fix exp=0 falsy guard (use !== undefined) - Release JWKS eviction lock on fetch error (add cacheDelete + try/finally) - Return 400 for malformed finalTarget URL instead of throwing TypeError - Fill in empty error message in composeMiddleware config guard - Align tenantId docs to string-only (function form deferred) - Add del() to CacheProvider interface and MemoryCacheProvider - Annotate @firebase/util and _AuthEmulatorRefreshToken as internal/fragile - Remove self-referential PR comment and resolved TODO - Centralize createToken test helper in middleware_test_utils.ts

Introduces the firebase-cookie-middleware package (v0.0.1) for Next.js 14/15/16, taking over from PR FirebaseExtended#640 (James Daniels). Security fixes applied during takeover review: - Emulator explicit opt-in only (env var not auto-detected) - JWT signature failure sets refreshable=false to prevent bypass - Verify refreshed token before accepting it - SSRF: loopback validation on emulator host - CSRF: Origin check on POST/DELETE to /__cookies__ - CHIPS: SameSite=None on partitioned cookies - JWKS eviction lock released in finally block - Falsy-zero TTL: require ttlSeconds > 0 before caching - Proxy targets validated; return 400 instead of throwing - Optional chaining on JWT payload access - Removed duplex: 'half' workaround (unnecessary in Node.js) Next.js 16: adds proxy export (proxy.ts replaces middleware.ts in Next.js 16 projects). proxy.ts runs on Node.js runtime only; middleware.ts remains available for Edge Runtime (deprecated). Also fixes pre-existing TypeScript strict-mode errors surfaced by upgrading moduleResolution from node to bundler. Ref: FirebaseExtended#640 Original work by: James Daniels (jamesdaniels@google.com)

tyler-reitz · 2026-07-16T22:52:18Z

/gemini review

gemini-code-assist · 2026-07-16T22:54:51Z

Warning

Gemini encountered an error creating the review. You can try again by commenting /gemini review.

The nextjs package is a self-contained sub-package with its own tsconfig and test runner. Including it in the root tsc/vitest runs causes failures because next is not installed as a root dependency.

Use **/ prefix so the patterns match at any directory depth, covering both src/nextjs/ and package/src/nextjs/ (the CI test job unpacks the npm tarball alongside the checkout, creating both paths). Also broaden node_modules exclusion to catch nested directories like functions/node_modules/.

tyler-reitz · 2026-07-16T23:03:33Z

/gemini review

gemini-code-assist · 2026-07-16T23:06:09Z

Warning

Gemini encountered an error creating the review. You can try again by commenting /gemini review.

tyler-reitz · 2026-07-16T23:15:06Z

/gemini review

gemini-code-assist · 2026-07-16T23:17:19Z

Warning

Gemini encountered an error creating the review. You can try again by commenting /gemini review.

…e throw with logout Three uncaught exceptions that crash middleware with 500: - proxy path: response.json() fired before response.ok check, returns 502 on non-JSON - refresh path: refreshResponse.json() now catches non-JSON bodies and calls logout() - refresh path: missing id_token now calls logout() instead of throwing Also remove duplicate JSDoc paragraph and fix two em-dashes in comments.

tyler-reitz · 2026-07-17T20:49:29Z

Local code review (in lieu of Gemini bot)

The Gemini review bot has failed on every attempt for this PR (5+ runs). The diff is 2,500+ lines of all-new code, which appears to exceed the bot's input limit. I ran a local multi-angle review as a substitute. Three findings were fixed in the latest commit (bb006e9); the rest are documented below for your consideration.

Fixed in `bb006e9`

1. Proxy path: response.json() before response.ok check (line 447)
response.json() was called unconditionally before response.ok was tested. Any non-JSON response from Firebase (CDN error page, 429 with text body) would throw an uncaught exception, crashing the middleware with a 500 for all users. Now wrapped in try/catch; returns 502 on non-JSON.

2. Refresh path: refreshResponse.json() uncaught (line 552)
Same pattern. If Google's token endpoint returns non-JSON (e.g., a CDN error page with status 200), the middleware crashed. Now calls logout() on parse failure.

3. Refresh path: throw instead of return logout() on missing id_token (line 555)
throw new Error("Missing id_token in refresh response") was uncaught and crashed the middleware. Replaced with console.error + return logout().

Remaining findings (for your review)

Security:

Line 244: jwtVerify has no issuer/audience options. Jose does not cryptographically enforce iss or aud; the manual pre-checks on the decodeJwt output cover these, but passing { issuer, audience } to jwtVerify would add a second verification layer. Low severity since the manual checks exist, but worth the defense-in-depth.
Line 362: CSRF guard only covers POST and DELETE. GET requests to /__cookies__ bypass the origin check and reach finalTarget parsing. Google's endpoints do not process GET token requests, so exploitability is low, but the guard intent is clearly "state-changing requests" and GET slips through.

Operational correctness:

Line 164: MAX_MAX_AGE is defined but not applied to JWKS caching. parseInt(match[1], 10) on the Cache-Control: max-age= value is used directly as the cache TTL. A server returning max-age=999999999 would cache JWKS for ~31 years in a custom CacheProvider, preventing key rotation. The constant MAX_MAX_AGE = 34560000 (400 days) is used for cookies but not here. Fix: Math.min(maxAge, MAX_MAX_AGE).
Line 688: If innie throws, all cookie decorators are discarded. The decorators.reduce at line 689 runs after innie. If the user's callback throws, accumulated logout and token-refresh decorators are silently lost, leaving stale cookies in the browser. Consider wrapping innie in try/catch and still running decorators on any fallback response.
Line 326: useInsecureCookies only activates for localhost/127.0.0.1. Developers on non-loopback http:// addresses (Docker at http://192.168.1.x:3000, remote dev VMs) get Secure; SameSite=None cookies that Chrome silently rejects on non-secure non-loopback origins. Auth silently fails with no error. Consider documenting this constraint prominently or expanding the loopback check.
Line 685: Multi-app: decorators from other apps applied to direct responses. When app1 returns a direct proxy response for /__cookies__?appName=app1 and app2 produces a logout decorator, the app2 decorator is applied to app1's JSON proxy response. The Firebase SDK reading app1's response may see unexpected Set-Cookie deletion headers. Consider skipping decorators for apps other than the one that produced the direct response.
Line 637: Array config always sets defaultAppName to '[DEFAULT]'. When composeMiddleware receives an array (as opposed to a single object), defaultAppName is hardcoded to '[DEFAULT]'. If no app in the array has appName: '[DEFAULT]', defaultUser in the innie callback is always undefined even after successful sign-in. The user must look up allUsers[appName] manually with no warning. Worth documenting or auto-detecting the first app as default.

Minor correctness:

Line 355: Empty appName= silently routes to [DEFAULT]. searchParams.get('appName') || '[DEFAULT]' treats an empty string the same as absent, misrouting in multi-app setups.
Line 313: /__/ rewrite does not guard against authDomain matching the current host. A misconfigured authDomain equal to the deployed domain creates an infinite rewrite loop on platforms that re-invoke middleware on rewrites (not standard Vercel/Next.js, but worth a guard).

Forward compatibility:

Line 433: request.cookies.get() called with a full cookie-descriptor object (spread of REFRESH_TOKEN_COOKIE including secure, httpOnly, sameSite, partitioned) rather than the documented string | { name } shape. Works today because Next.js ignores the extra fields, but could break silently on a future Next.js release that validates the argument.

Overall the package is solid. The three crash-on-error cases are fixed; the remaining items are defense-in-depth or edge-case correctness issues appropriate for follow-up issues.

Adds 'import' condition alongside 'module' so TypeScript moduleResolution bundler resolves the package types correctly. Switches tsconfig from noEmit to emitDeclarationOnly so tsc generates index.d.ts in dist/.

armando-navarro

I ran the suite (71/71 green), confirmed the sub-package is fully isolated from the reactfire build/tsc/publish, and read the middleware closely. One thing I found looks worth fixing before this lands, plus a couple of smaller items.

Refresh path is missing the main-path guard against unsigned tokens

The description lists "verify-after-refresh" as a fix, so I went looking at how the refreshed token is validated. My reading is that the refresh path doesn't carry the same alg:"none" guard the main path has: an unsigned token in the refresh response is accepted where the identical token would be rejected if it arrived as the ID-token cookie. To be clear up front, I don't think this is remotely exploitable today (reasoning below); it reads to me as a defense-in-depth gap in the stated fix rather than a live hole.

The asymmetry:

verifyFirebaseIdToken treats an alg:"none" header as an emulator credential and returns the payload without a signature check (index.ts:219-230). That's by design, since there's no signature to verify. The safety check is meant to be the caller inspecting the returned isEmulatedCredential flag.
The main entry path does inspect it: if (isEmulatedCredential && !options.emulatorHost) return logout().
The refresh path discards it: const [verifiedNewPayload] = await verifyFirebaseIdToken(...) and then accepts the payload, with no emulatorHost re-check.

I reproduced it locally with a throwaway test: mock jose.jwtVerify to always reject (so anything accepted must have skipped signature checking), and have the refresh endpoint return an alg:"none" token with well-formed claims. The refresh path accepted it as an authenticated user; the identical token as the ID cookie was rejected.

To be honest about exploitability: reaching this in production sends the refresh to the hardcoded securetoken.googleapis.com, which you don't control, so over intact HTTPS to Google it isn't remotely exploitable. It only bites if the refresh response is attacker-influenced (TLS interception, a compromised upstream, or a later refactor that varies the refresh target), which is the same "compromised token endpoint" case the verify-after-refresh fix is aimed at, so it seemed in-scope to me rather than a follow-up. Unless I'm misreading the intended boundary, mirroring the main-path guard would close it:

const [verifiedNewPayload, newIsEmulated] = await verifyFirebaseIdToken(newIdToken, ...);
if (!verifiedNewPayload || (newIsEmulated && !options.emulatorHost)) return logout();

A regression test for an alg:"none" token arriving via refresh would be worth adding alongside, since middleware_refresh.node.test.ts currently only covers signed tokens and emulator-refresh-token detection, so this path isn't exercised.

Smaller items

README SameSite (README.md:218): the Cookie Architecture section says the ID-token cookie is SameSite=Lax, but production sets SameSite=None (line 333); Lax is only the Safari-localhost fallback, and CHIPS Partitioned requires None anyway. The code's None is deliberate and correct for partitioned cookies, so this is purely a docs fix, but worth getting right because it matters to the security model: with SameSite=None the auth cookies are sent cross-site, so the Origin check in the /__cookies__ handler (362-367) is the actual CSRF defense, not SameSite. A reader trusting the README would assume a SameSite protection that isn't there.
JWKS TTL (index.ts:159-164): following up on your own thread note about the unclamped max-age. It only bites a custom CacheProvider that reads the cached JWKS back: the default cache's reads short-circuit to the remote JWKS set (line 144), so even though the rotation refetch path does write a firebase:jwks entry, the default cache never reads it. Add the ERR_JWKS_NO_MATCHING_KEY refetch self-healing on rotation and it's minor overall. One thing on the suggested Math.min(maxAge, MAX_MAX_AGE): MAX_MAX_AGE is the 400-day cookie bound, which is a much larger magnitude than a JWKS TTL, so if you do clamp it, a JWKS-appropriate ceiling is probably what you want. Your call whether it's worth it.

Everything else I poked at (emulator loopback pinning, the proxy target allow-list, the explicit emulator:true opt-in, the per-project cache-key safety) held up well. I may be missing something about how you intend the refresh boundary to work, so if the alg:"none" case is handled somewhere I overlooked, point me at it and I'll take another pass.

armando-navarro · 2026-07-18T05:26:32Z

+  }
+  // Full signature + claims verification on the refreshed token. Caching is
+  // handled inside verifyFirebaseIdToken, so no separate cacheSetEx needed.
+  const [verifiedNewPayload] = await verifyFirebaseIdToken(


This drops isEmulatedCredential from the tuple, so the refresh path has no equivalent of the main-path guard at line 501 (isEmulatedCredential && !options.emulatorHost → logout). An alg:"none" token in the refresh response is accepted here without a signature check, where the same token as a cookie would be rejected. Not exploitable over intact HTTPS to Google, but it's a gap in the "full verification" the comment above promises. Mirroring line 501 would close it:

const [verifiedNewPayload, newIsEmulated] = await verifyFirebaseIdToken(newIdToken, options.projectId, options.tenantId, cache); if (!verifiedNewPayload || (newIsEmulated && !options.emulatorHost)) { console.error("Refreshed token failed verification"); return logout(); }

Fixed in acbb278: mirrored the line 501 guard onto the refresh path (checks newIsEmulatedCredential && !options.emulatorHost before accepting the refreshed payload) and added a regression test for an alg:none token arriving via refresh. Thanks for the thorough repro.

The refresh path called verifyFirebaseIdToken but discarded the isEmulatedCredential flag, so an alg:none token in a refresh response was accepted without the same emulatorHost check the main path enforces at line 501. Mirrors that guard and adds a regression test. Reported by Armando in review.

README said SameSite=Lax; production actually sets SameSite=None (Lax is only the Safari-localhost fallback, since Partitioned cookies require None). Also notes that CSRF protection for /__cookies__ comes from the Origin check, not SameSite, since None sends the cookie cross-site. Flagged by Armando in review.

…ching A server-supplied max-age was used directly as the JWKS cache TTL with no ceiling, so a custom CacheProvider could cache keys for years and miss rotation. Adds a JWKS-sized 1-day ceiling instead of reusing the 400-day cookie MAX_MAX_AGE, which was the wrong order of magnitude for this case. Flagged by Armando in review.

armando-navarro

Thanks Tyler, these all look good. I pulled the branch (09091ed), reran the suite (73/73), and read the three changes.

Refresh-path guard

This mirrors the main-path check, and the flag now drives a logout before the payload is accepted or the cookie is set:

if (newIsEmulatedCredential && !options.emulatorHost) {
  return logout();
}

I checked the new regression test is actually load-bearing by removing the guard and rerunning: the middleware_refresh test fails (the alg:"none" refresh token gets accepted) and passes again once the guard is back. That's exactly the asymmetry closed.

README SameSite

Reads correctly now: SameSite=None with Lax as the Safari-localhost fallback, plus the note that the Origin check is the CSRF defense on /__cookies__. Matches the code.

JWKS max-age clamp

Introducing MAX_JWKS_MAX_AGE (1 day) rather than reaching for the 400-day cookie bound is the right call, and the test confirms an oversized max-age clamps to 86400.

Approving. If I've misjudged any of the boundaries here, point me at it and I'll take another look.

V5 targets React 18+ which ships useSyncExternalStore natively. Importing from use-sync-external-store/shim (a CJS module) caused rolldown to inline a CJS require shim into the ESM dist, which throws at runtime in Next.js App Router (no require available in ESM context).

tyler-reitz · 2026-07-21T20:25:30Z

Ran the sandbox-nextjs V5 demo app against this branch end-to-end (auth middleware, Firestore collection with security rules, storage upload). Two library-level findings:

Fixed on this branch: useObservable was importing useSyncExternalStore from use-sync-external-store/shim (a CJS module). rolldown inlines the CJS content into the ESM dist, which puts a bare require('react') call in the output. Next.js App Router runs the dist as native ESM and throws at load time. Changed the import to 'react' directly since V5 targets React 18+ where the API is native (commit e98628c).

Outstanding: useObservable calls useSyncExternalStore(subscribe, getSnapshot) without a third getServerSnapshot argument. React 18 requires it for server rendering and throws when it's absent. During testing we observed Next.js catch this and fall back gracefully, so the app works, but we didn't characterize exactly what the server sends in that case. The arch review flagged this as a V5 "just do it" item. Tracking in #748.

jamesdaniels · 2026-07-21T20:42:20Z

@tyler-reitz thanks for taking to the finish line. duplex half helps in cloudflare / vercel edge FYI.

tyler-reitz mentioned this pull request Jul 16, 2026

NextJS middleware #640

Closed

jamesdaniels and others added 6 commits July 16, 2026 15:51

First.

17e4e70

Working on the middleware behavior a bit more

9a3b590

Taking it further

6b3abcb

Handle targetAppName

4b188ab

tyler-reitz force-pushed the feat/nextjs-middleware branch from 40b253c to 37050fa Compare July 16, 2026 22:51

tyler-reitz added 2 commits July 16, 2026 15:56

chore: exclude src/nextjs from root tsc and vitest

fffb237

The nextjs package is a self-contained sub-package with its own tsconfig and test runner. Including it in the root tsc/vitest runs causes failures because next is not installed as a root dependency.

chore: remove src/nextjs/package-lock.json from tracking

048987a

tyler-reitz requested a review from armando-navarro July 17, 2026 21:16

tyler-reitz force-pushed the feat/nextjs-middleware branch from 020570a to bb006e9 Compare July 17, 2026 22:09

fix(nextjs): add import condition to exports map, emit type declarations

809be87

Adds 'import' condition alongside 'module' so TypeScript moduleResolution bundler resolves the package types correctly. Switches tsconfig from noEmit to emitDeclarationOnly so tsc generates index.d.ts in dist/.

armando-navarro requested changes Jul 18, 2026

View reviewed changes

tyler-reitz added 3 commits July 20, 2026 14:25

tyler-reitz requested a review from armando-navarro July 20, 2026 21:28

armando-navarro approved these changes Jul 21, 2026

View reviewed changes

tyler-reitz requested review from armando-navarro and jhuleatt July 21, 2026 18:35

tyler-reitz mentioned this pull request Jul 21, 2026

useObservable: add getServerSnapshot to useSyncExternalStore for SSR #748

Open

Uh oh!

Conversation

tyler-reitz commented Jul 16, 2026

Summary

Test plan

Attribution

Uh oh!

gemini-code-assist Bot commented Jul 16, 2026

Uh oh!

tyler-reitz commented Jul 16, 2026

Uh oh!

gemini-code-assist Bot commented Jul 16, 2026

Uh oh!

tyler-reitz commented Jul 16, 2026

Uh oh!

gemini-code-assist Bot commented Jul 16, 2026

Uh oh!

tyler-reitz commented Jul 16, 2026

Uh oh!

gemini-code-assist Bot commented Jul 16, 2026

Uh oh!

tyler-reitz commented Jul 16, 2026

Uh oh!

gemini-code-assist Bot commented Jul 16, 2026

Uh oh!

tyler-reitz commented Jul 17, 2026

Local code review (in lieu of Gemini bot)

Fixed in bb006e9

Remaining findings (for your review)

Uh oh!

armando-navarro left a comment

Choose a reason for hiding this comment

Refresh path is missing the main-path guard against unsigned tokens

Smaller items

Uh oh!

armando-navarro Jul 18, 2026

Choose a reason for hiding this comment

Uh oh!

tyler-reitz Jul 20, 2026

Choose a reason for hiding this comment

Uh oh!

armando-navarro left a comment

Choose a reason for hiding this comment

Refresh-path guard

README SameSite

JWKS max-age clamp

Uh oh!

tyler-reitz commented Jul 21, 2026

Uh oh!

jamesdaniels commented Jul 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Fixed in `bb006e9`